feat: add ViewModel progress infrastructure [DEV] by criticalAY · Pull Request #20729 · ankidroid/Anki-Android

criticalAY · 2026-04-12T14:42:51Z

Purpose / Description

Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.

Fixes

Fixes part of architecture: support withProgress in ViewModels #19460

Approach

The ViewModel exposes a state flow. The Activity/Fragment calls observeProgress(viewModel) once, and the LoadingDialogFragment is shown/dismissed automatically based on state changes.
Cancellation follows the same pattern as the old API (pass an onCancel callback to withProgress). The observer wires it to the dialog's cancel listener. (I actually followed a different pattern here then I saw David's comment on original issue)

How Has This Been Tested?

Unit test

Learning (optional, can help others)

NA

Checklist

Please, go through these checks before submitting the PR.

You have a descriptive commit message with a short title (first line, max 50 chars).
You have commented your code, particularly in hard-to-understand areas
You have performed a self-review of your own code
UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
UI Changes: You have tested your change using the Google Accessibility Scanner

david-allison

This looks exceptional. One question on the race condition

david-allison · 2026-04-19T07:08:55Z

+    /** The cancel callback for the currently active cancellable operation, if any */
+    @Volatile
+    private var currentOnCancel: (() -> Unit)? = null


I feel we should store each onCancel.

This would require arbitrary access to the list of currently active methods, as they are removed based on the completion time.

david-allison · 2026-04-19T07:19:37Z

+            val mutex = Mutex()
+            var showJob: Job? = null
+            var dialogVisible = false
+            viewModel.progressManager.progress.collect { state ->


Consider using launchCollectionInLifecycleScope

david-allison · 2026-04-19T07:19:39Z

+        repeatOnLifecycle(Lifecycle.State.STARTED) {
+            val mutex = Mutex()
+            var showJob: Job? = null
+            var dialogVisible = false


Sorry... I was incorrect here. You need a lock when updating the progress state, but this isn't an update method.

dialogVisible doesn't restore state from the FragmentManager

david-allison · 2026-04-19T07:29:31Z

+        activeCount.incrementAndGet()
+        if (onCancel != null) {
+            currentOnCancel = onCancel
+        }
+        updateState(message = message, amount = null, cancellable = onCancel != null)
+        try {
+            return ProgressScope(this).block()
+        } finally {
+            if (onCancel != null) {
+                currentOnCancel = null
+            }
+            if (activeCount.decrementAndGet() == 0) {
+                progress.value = ViewModelProgress.Idle
+            }
+        }


Concurrency bugs here: There's dependencies between currentOnCancel, activeCount and progress, but locks aren't held to serialise the mutations.

david-allison · 2026-04-19T07:36:10Z

Thinking more:

What's does progress/cancellation mean if there are two concurrent progress operations.

Let's define this explicitly, both the UI and UX.

BrayanDSO · 2026-04-23T07:23:42Z

+    data object Idle : ViewModelProgress
+
+    /**
+     * One or more operations in progress.


The doc in the interface, message param and cancellable param say operation (singular), while this says One or more (plural). What's the actual supported behavior? Update that on the docs

BrayanDSO · 2026-04-23T07:45:00Z

+ * `StateFlow` re-emissions on re-resume don't re-run [block] unnecessarily.
+ */
+context(activity: AnkiActivity)
+fun <T> Flow<T>.launchLatestCollectionInLifecycleScope(block: suspend (T) -> Unit) {


I don't like the method as a general util method, although it's quite similar to some methods above it.

Name is too big, the manual value skipping can be unexpected, and doing that many things inside a single method can lead to bugs.

BrayanDSO · 2026-04-23T07:47:38Z

+    viewModel: HasProgress,
+    delayMillis: Duration = 600.milliseconds,
+) {
+    (requireActivity() as AnkiActivity).observeProgress(viewModel, delayMillis)


Can throw a ClassCastException. I guess that none of the other methods actually need an AnkiActivity and could use a FragmentActivity instead. Either that or handle the throw

That would require change in LoadingDialogFragment leaving a TODO for follow ups

BrayanDSO · 2026-04-23T07:48:56Z

+private fun formatProgressMessage(state: ViewModelProgress.Active): String? {
+    val amount = state.amount
+    if (amount != null && state.message != null) {
+        return "${state.message} ${amount.current}/${amount.max}"
+    }
+    return state.message
+}


it would be nice to be able to add a custom format instead of being restricted to "${state.message} ${amount.current}/${amount.max}"

Ref:

Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/CoroutineHelpers.kt

Lines 543 to 555 in d529ff1

/**

* Holds the current backend progress, and text/amount properties

* that can be written to in order to update the UI.

*/

data class ProgressContext(

var progress: Progress = Progress.getDefaultInstance(),

var text: String? = null,

/** If set, shows a progress bar with `current` of `max` complete. */

var amount: Amount? = null,

val formatAmount: (Amount) -> String = { (current, max) -> "$current/$max" },

/** Separator between [text] and [amount] */

val separator: String = " ",

) {

Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.

github-actions Bot added the Needs Review label Apr 12, 2026

criticalAY force-pushed the feat/withprogress branch 2 times, most recently from b6fabdb to c278581 Compare April 12, 2026 14:47

david-allison reviewed Apr 15, 2026

View reviewed changes

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/progress/ProgressManager.kt Outdated

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/progress/ProgressObserver.kt Outdated

Comment thread AnkiDroid/src/main/java/com/ichi2/anki/progress/ProgressObserver.kt Outdated

david-allison added the Needs Author Reply Waiting for a reply from the original author label Apr 15, 2026

criticalAY force-pushed the feat/withprogress branch from c278581 to d97aee6 Compare April 15, 2026 13:23

criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Apr 15, 2026

criticalAY requested a review from david-allison April 15, 2026 13:24

criticalAY added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Apr 17, 2026

criticalAY force-pushed the feat/withprogress branch from d97aee6 to 97cb3d7 Compare April 18, 2026 18:19

criticalAY added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Apr 18, 2026

david-allison requested changes Apr 19, 2026

View reviewed changes

criticalAY force-pushed the feat/withprogress branch 2 times, most recently from 416c3a0 to 79d16a6 Compare April 21, 2026 09:53

david-allison mentioned this pull request Apr 23, 2026

feat(study-screen): Reset progress #20822

Merged

5 tasks

BrayanDSO reviewed Apr 23, 2026

View reviewed changes

feat: add ViewModel progress infrastructure

3656563

Introduce a common pattern for progress notifications in ViewModels, decoupling progress dialog management from Activity/Fragment context.

criticalAY force-pushed the feat/withprogress branch from 79d16a6 to 3656563 Compare April 24, 2026 11:43

	/**
	* Holds the current backend progress, and text/amount properties
	* that can be written to in order to update the UI.
	*/
	data class ProgressContext(
	var progress: Progress = Progress.getDefaultInstance(),
	var text: String? = null,
	/** If set, shows a progress bar with `current` of `max` complete. */
	var amount: Amount? = null,
	val formatAmount: (Amount) -> String = { (current, max) -> "$current/$max" },
	/** Separator between [text] and [amount] */
	val separator: String = " ",
	) {

Uh oh!

Conversation

criticalAY commented Apr 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Learning (optional, can help others)

Checklist

Uh oh!

david-allison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

david-allison Apr 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-allison commented Apr 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-allison Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

criticalAY commented Apr 12, 2026 •

edited

Loading

david-allison Apr 19, 2026 •

edited

Loading

david-allison Apr 23, 2026 •

edited

Loading